Saves computed styles in the state #9176

NejcZdovc · 2017-05-31T16:40:07Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Resolves #9149

Auditors: @bsclifton @bridiver

Test Plan:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

bridiver · 2017-06-07T17:38:08Z

app/renderer/components/window.js

@@ -93,11 +96,14 @@ class Window extends React.Component {

  componentDidMount () {
    appActions.windowReady(getCurrentWindowId())
+    this.updateComputedStyles()
+    window.addEventListener('resize', this.updateComputedStyles)


we already have a window resize handler in windows.js that triggers appActions.windowUpdated and it is debounced to prevent a flood of actions during resize. I think it would be better to send that action to the window using queryInfo (see urlBarTextChanged for an example) and update the computed styles in response to that

bridiver · 2017-06-07T17:40:03Z

app/renderer/components/window.js

@@ -45,6 +47,7 @@ class Window extends React.Component {

    this.onChange = this.onChange.bind(this)
    this.onAppStateChange = this.onAppStateChange.bind(this)
+    this.updateComputedStyles = throttle(this.updateComputedStyles.bind(this), 66)


I think we can get rid of this and rely on the existing windowUpdated debounce

Moved into windows.js

bridiver · 2017-06-07T17:42:27Z

js/actions/appActions.js

@@ -1411,6 +1411,14 @@ const appActions = {
    dispatch({
      actionType: appConstants.APP_UPDATE_LOG_OPENED
    })
+  },
+
+  updateComputedStyles: function (windowId, values) {


I think this should actually be in windowActions. We are generally moving away from windowActions and appActions, but we will keep windowActions for actions that are related to the window component. It can still be handled in the browser process

also should be declarative updateComputedStyles -> computedStylesChanged or something similar

bridiver · 2017-06-07T17:47:52Z

app/common/state/windowState.js

@@ -148,6 +148,13 @@ const api = {
    return state.set('windows', windows.delete(index).insert(index, windowValue))
  },

+  updateComputedStyles: (state, action) => {


I think setComputedStyles and we are moving away from passing actions into state helpers so I think this should take windowId and computedStyles

bridiver · 2017-06-07T17:51:17Z

app/common/lib/windowsUtil.js

+  return result
+}
+
+const getComputedProperty = (state, property) => {


I think this should be replaced by a getComputedStyles method in windowState. We don't want anything outside of windowState to be tightly coupled to the storage path

bridiver · 2017-06-07T17:51:59Z

I like the general idea, just needs some tweaks

NejcZdovc · 2017-06-13T12:30:54Z

app/browser/windows.js

    appActions.windowUpdated(windowValue)
+    updateComputedStyles(win)


The only problem with calling this function from this file is that we don't have document, so you can't do document.querySelector. Any idea how to fix this? @bridiver

NejcZdovc · 2017-06-15T07:59:20Z

@bridiver can you please checkout my review comment

bridiver · 2017-06-15T21:33:47Z

app/renderer/components/bookmarks/bookmarksToolbar.js

-      this.chevronWidth = this.chevronMargin + Number.parseInt(this.fontSize)
-    }
+    this.maxWidth = windowsUtil.getCSSElementProperty('bookmark-item-max-width')
+    this.padding = windowsUtil.getCSSElementProperty('--bookmark-item-padding') * 2


these shouldn't have dashes, right?

You are correct. Fixed

bridiver

lgtm, maybe just some naming changes

bridiver · 2017-06-15T21:48:59Z

app/common/constants/computedStyles.js

+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const computedStyles = {


should this go under renderer now instead of common?

computed also probably doesn't make sense anymore since these are static values

oh, almost forgot. We should comment this file and the less files to make sure they don't get out-of-sync

don't want anyone to think they can only update one or the other

I would leave it here, because we have all constants files here and this are constants

Comment added

bridiver · 2017-06-15T21:50:38Z

app/common/lib/windowsUtil.js

  return Immutable.fromJS({
    location: site.get('location'),
    order: site.get('order'),
    partitionNumber: site.get('partitionNumber') || 0
  })
 }
+
+const getCSSElementProperty = (prop) => {


maybe renderer/styleUtil ? getStyleValue ?

bridiver · 2017-06-15T22:18:13Z

please add comments to styleValues.js and relevant less files in a follow-up. Thanks!

Resolves brave#9149 Auditors: Test Plan:

NejcZdovc added the refactoring label May 31, 2017

NejcZdovc added this to the 0.18.x milestone May 31, 2017

NejcZdovc self-assigned this May 31, 2017

NejcZdovc requested review from bridiver and bsclifton and removed request for bridiver and bsclifton May 31, 2017 16:40

NejcZdovc added the PR/work-in-progress ⚒ label May 31, 2017

NejcZdovc force-pushed the refactor/#9149-computed branch from 3e4eabf to 451cf04 Compare June 1, 2017 17:07

This was referenced Jun 3, 2017

Redux refactoring tracking issue #8528

Closed

Converts DownloadItem and DownloadsBar into redux #9271

Merged

bridiver suggested changes Jun 7, 2017

View reviewed changes

NejcZdovc mentioned this pull request Jun 7, 2017

Converts urlBarSuggestions into redux #9051

Merged

7 tasks

NejcZdovc force-pushed the refactor/#9149-computed branch 5 times, most recently from dae69f6 to a4213bc Compare June 13, 2017 12:29

NejcZdovc commented Jun 13, 2017

View reviewed changes

NejcZdovc mentioned this pull request Jun 13, 2017

Converts bookmarksToolbar and bookmarkToolbarButton into redux #9030

Merged

7 tasks

NejcZdovc force-pushed the refactor/#9149-computed branch from a4213bc to bf7c248 Compare June 13, 2017 15:49

NejcZdovc modified the milestones: 0.19.x (Nightly Channel), 0.18.x (Developer Channel) Jun 13, 2017

NejcZdovc force-pushed the refactor/#9149-computed branch 2 times, most recently from ca1a1a1 to 44c65ae Compare June 15, 2017 21:31

NejcZdovc removed the PR/work-in-progress ⚒ label Jun 15, 2017

NejcZdovc requested a review from bridiver June 15, 2017 21:31

bridiver reviewed Jun 15, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9149-computed branch from 44c65ae to c1cfe7e Compare June 15, 2017 21:45

NejcZdovc requested a review from bridiver June 15, 2017 21:46

bridiver approved these changes Jun 15, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9149-computed branch 2 times, most recently from cd4775a to 2c1c1be Compare June 15, 2017 22:09

Saves computed styles in the state

e2df69a

Resolves brave#9149 Auditors: Test Plan:

NejcZdovc force-pushed the refactor/#9149-computed branch from 2c1c1be to e2df69a Compare June 16, 2017 08:13

NejcZdovc merged commit f1b2bb8 into brave:master Jun 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Saves computed styles in the state #9176

Saves computed styles in the state #9176

NejcZdovc commented May 31, 2017 •

edited

Loading

bridiver Jun 7, 2017

NejcZdovc Jun 13, 2017

bridiver Jun 7, 2017

NejcZdovc Jun 13, 2017 •

edited

Loading

bridiver Jun 7, 2017

bridiver Jun 7, 2017

NejcZdovc Jun 13, 2017

bridiver Jun 7, 2017

NejcZdovc Jun 13, 2017

bridiver Jun 7, 2017

NejcZdovc Jun 13, 2017

bridiver commented Jun 7, 2017

NejcZdovc Jun 13, 2017

NejcZdovc commented Jun 15, 2017

bridiver Jun 15, 2017

NejcZdovc Jun 15, 2017

bridiver left a comment

bridiver Jun 15, 2017

bridiver Jun 15, 2017

bridiver Jun 15, 2017

bridiver Jun 15, 2017

NejcZdovc Jun 15, 2017

NejcZdovc Jun 15, 2017

bridiver Jun 15, 2017

bridiver commented Jun 15, 2017

		appActions.windowUpdated(windowValue)
		updateComputedStyles(win)

Saves computed styles in the state #9176

Saves computed styles in the state #9176

Conversation

NejcZdovc commented May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver commented Jun 7, 2017

Choose a reason for hiding this comment

NejcZdovc commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver commented Jun 15, 2017

NejcZdovc commented May 31, 2017 •

edited

Loading

NejcZdovc Jun 13, 2017 •

edited

Loading